Skip to content

Conversation

@rekmarks
Copy link
Member

@rekmarks rekmarks commented Aug 7, 2024

Extracted from #11
Requires #15

This introduces @ocap/streams, a package that exports @endo/captp-compatible streams over the browser MessageChannel / MessagePort transport mechanism. These streams replace the existing window.postMessage() transport mechanism used in the testbed extension. @endo/captp will be introduced in a subsequent PR.

Since constructing its streams requires the existence of ports, @ocap/streams also establishes a simple protocol for creating a MessageChannel between a window and one of its iframes. To engage in this protocol, the parent calls initializeMessageChannel() and the child calls receiveMessagePort(). See message-channel.ts for more details.

In order to make use of the Error cause feature (ref), bumps the default TypeScript lib option from ES2020 to ES2022. Although this is incompatible with the current minimum Chrome version of the MetaMask extension, it will more than likely be raised by the time we need it for a production release.

@socket-security
Copy link

socket-security bot commented Aug 7, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@endo/stream@1.2.4 None 0 33 kB kriskowal
npm/@types/jsdom@21.1.7 None +2 737 kB types
npm/typescript@4.9.5 None 0 66.8 MB typescript-bot

🚮 Removed packages: npm/typescript@5.5.4

View full report↗︎

@rekmarks rekmarks force-pushed the rekm/monorepo-chores branch from 46600ea to c2a46ae Compare August 7, 2024 15:59
@rekmarks rekmarks force-pushed the rekm/streams-2 branch 5 times, most recently from 641c30f to 031a8b8 Compare August 7, 2024 18:25
rekmarks added a commit that referenced this pull request Aug 9, 2024
In the course of implementing #11, I made various changes to the
monorepo that I deemed either necessary or simply beneficial. Here these
changes are extracted from #11, which has been separated into this PR
and #16. Contains the following changes:

- Upgrade `ts-bridge` and migrate all packages to ESM
- In the course of implementing #11, I ran into a [TypeScript project
references](https://www.typescriptlang.org/docs/handbook/project-references.html)-related
error, because [`ts-bridge`](https://ts-bridge.dev/) originally did not
support this feature. However, [`ts-bridge` recently added support for
project
references](https://github.com/ts-bridge/ts-bridge/releases/tag/v7.0.0),
and updating fixed the problem.
- This, however, necessitated migrating all monorepo packages to be
ESM-first (`"type": "module"`), and setting TypeScript's
`moduleResolution` algorithm to `Node16`. So, now the entire monorepo is
ESM-first, but any libraries we publish will still be importable in
CommonJS, due to `ts-bridge`.
- As part of this change, `.js` files were renamed to `.cjs`, and `.mjs`
files to `.js`. The same goes for their `.ts` equivalents.
- A notable exception includes the published files in `@ocap/shims`,
which retain their `.mjs` extensions as a redundancy measure.
- Fix intra-repo path resolution in tests
- In the `/snaps` and `/core` monorepos, Jest's `moduleNameMapper`
option is manually configured to match the `paths` property in
`tsconfig.json` to ensure that tests use source code as opposed to build
outputs.
- In `vite`, we accomplish the same by dropping in the plugin
`vite-tsconfig-paths`, with no additional configuration whatsoever.
- Add internal `test-utils` package
- Rather than keeping a folder of test utilities at the monorepo root, I
decided to just create an internal package for this purpose.
- Set the version of all intra-monorepo dependencies to `workspace:^`
- This takes a queue from the Snaps monorepo. In this way, we will never
accidentally pull a workspace package from npm.
- Note that this required stealing some of the Snaps monorepo's
constraints for our `constraints.pro`. However, don't spend too much
time on that file, because it will be replaced soon: #17
- Externalize `endoify.mjs` in `extension` HTML files
- Introduces our own tiny `vite` plugin, `endoifyHtmlFilesPlugin`, that
inserts the `endoify.mjs` in the correct place just before HTML files
are built. `vite`'s aggressive optimization and module resolution logic
otherwise prevents us from doing this.
- Add `clean` script to all packages
- After experiencing some issues with cached builds, I decided to make
sure every package has a `clean` script.
Base automatically changed from rekm/monorepo-chores to main August 9, 2024 16:01
@rekmarks rekmarks changed the title feat: Add streams package feat: Add streams package Aug 9, 2024
@rekmarks rekmarks marked this pull request as ready for review August 9, 2024 17:47
@rekmarks rekmarks requested a review from a team as a code owner August 9, 2024 17:47
@grypez grypez self-requested a review August 13, 2024 03:12
Copy link
Contributor

@grypez grypez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed the newly added streams package but not the edits to the extension package. Overall, the message-channel looks like it well handles the issue of getting messages to the iframes.

The resolve-only stream implementations give me pause, but I can imagine the errors being handled at a higher level of abstraction as you indicate. I would only ask if the inability of the stream to detect its internal failure relieves it of the responsibility to support async throw (see the endo makeStream implementation for comparison). If indeed it does, then I recommend removing the reject part of the promise from the MessagePortReader implementation altogether.

@rekmarks

This comment was marked as resolved.

@rekmarks
Copy link
Member Author

rekmarks commented Aug 19, 2024

@grypez rather than removing reject, I added error handling, which makes use of it. See cc33a18, including the commit message.

The assumption that the MessagePort transport mechanism is completely wrong, as illustrated by this gist.

@rekmarks rekmarks requested a review from grypez August 19, 2024 11:53
The streams originally did not have any error handling capabilities,
since the underlying MessagePort was assumed to be completely reliable.
This was a bad asssumption, because `MessagePort.postMessage()` does in
fact throw DOMException errors under various conditions. These
conditions are not necessarily plausible, but we should handle the
possibility nevertheless.

Therefore, the streams now include error handling, and throwing a writer
will cause the reader on the opposite side to throw as well.
Bumps the default TypeScript lib for all packages from ES2020 to ES2022.
This is not currently compatible with the minimum Chrome version used in
the MetaMask extension (v89), but the target version (v93, ref:
https://caniuse.com/?feats=mdn-javascript_builtins_array_at,mdn-javascript_builtins_regexp_hasindices,mdn-javascript_builtins_object_hasown,mdn-javascript_builtins_error_cause,mdn-javascript_operators_await_top_level,mdn-javascript_classes_private_class_fields,mdn-javascript_classes_private_class_methods,mdn-javascript_classes_static_class_fields,mdn-javascript_classes_static_initialization_blocks)
is only a few versions ahead, and likely to be below the minimum by the
time the Ocap Kernel makes it to production.
@rekmarks
Copy link
Member Author

rekmarks commented Aug 19, 2024

@grypez I just rebased on latest main. See b1d30df and on for relevant changes. 28f3659 is merely lint-related fixes following the rebase.

Copy link
Contributor

@grypez grypez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rekmarks rekmarks merged commit f5d5c4c into main Aug 20, 2024
@rekmarks rekmarks deleted the rekm/streams-2 branch August 20, 2024 06:54
SMotaal added a commit that referenced this pull request Aug 22, 2024
SMotaal added a commit that referenced this pull request Aug 22, 2024
This bumps `typescript@5.5.4` once again, reapplying changes from #23
which were accidentally reverted in #16.

This also applies minor patches to `packages/shims/scripts/bundle.js` to
prevent a known failure in `node@18`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants